-
Notifications
You must be signed in to change notification settings - Fork 6
[CDF-26465] Rebuild build #2273
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not part of the task, strictly speaking. Just a proposal from the AI to improve performance through compiling regex'es. Will split into separate task later, please ignore for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you mean it will not be part of this PR?
If you want to do that performance boost, can you check that it is actually a boost? Introducing threading and locks increases the complexity of the code.
doctrino
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have done a first pass and thrown in some comments as I went through. I think I understand the code, but I am not seeing completely were it is heading. But more than good enough to start a discussion :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good refactoring that can be moved out to a separate PR.
☂️ Python Coverage
Overall Coverage
New Files
Modified Files
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2273 +/- ##
==========================================
+ Coverage 84.63% 84.66% +0.03%
==========================================
Files 281 284 +3
Lines 27762 27971 +209
==========================================
+ Hits 23496 23683 +187
- Misses 4266 4288 +22
🚀 New features to boost your workflow:
|
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a significant refactoring of the build command, creating a new v2 implementation behind a feature flag. The changes are well-structured, moving validation logic into a dedicated function and introducing Pydantic models for build inputs and issues, which aligns with the repository's style guide. The performance of variable replacement is also improved by caching compiled regular expressions.
My main concern is that the new v2 build command appears to have lost support for packages defined in cdf.toml, which is a functional regression. I've left a comment with a suggestion on how to restore this functionality.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
doctrino
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall good structure, a few clarifying questions and suggestions.
|
|
||
| cmd = BuildCommand(print_warning=print_warning) | ||
| cmd = ( | ||
| BuildCommandV2(print_warning=print_warning) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will you be able to reuse the old CLI interface? I would expect that to change, but I might be mistaken.
| description: str | ||
|
|
||
|
|
||
| class BuildIssueList(RootModel[list[BuildIssue]]): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if you subclass UserList you do not have to repeat all the standard list method (append, extend, len)
| warnings.append(environment_warning) | ||
| return config, warnings | ||
|
|
||
| @property |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good case for cached property?
| user_selected_modules = self.config.environment.get_selected_modules({}) | ||
| return ModuleDirectories.load(self.organization_dir, user_selected_modules) | ||
|
|
||
| @property |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good case for cached property?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file should not be committed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
| return f"{prefix}{suffix}" | ||
|
|
||
|
|
||
| def validate_module_selection( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a nice refactoring. As this have a lot of logic, it should have unit tests. Can you add a task for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you mean it will not be part of this PR?
If you want to do that performance boost, can you check that it is actually a boost? Introducing threading and locks increases the complexity of the code.
Description
The goal of this change is to improve the performance and overall user experience of the build command. It should also result in more maintainable code.
Flow
The general flow aims to collect and validate in stages, eventually doing more stuff in parallel and finally running recommendations and fixes on the built state. It might be better to run optimizations on individual modules, so this may change.
Structure
We'll need a new set of data classes/structure for recommendations. A recommendation will have a unique code so that we can link to docs, but also so that the user can dictate which rules and recommendation that they want to show (yes, like ruff and mypy) in their cdf.toml.
A recommendation, if fixable,
Collecting and logging warnings
Warnings should be collected and grouped by category so that we can print (or write to file) more readable logs;
Exit on warning still applies, but not before warnings have been printed or logged. Exceptions are raised as usual.
Testing
TBD. We'll need to do full regression testing here, in addition to adding new unit tests.
Release
This change will be behind a alpha flag until we decide to release it (planned v0.8)
Bump
Changelog
Added